fix: preserve ModelScope MCP transport#8270
Conversation
dosubot
Bot
added
size:S
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a helper function, _modelscope_mcp_transport_from_url_info, to dynamically determine the MCP transport type for ModelScope servers based on provided URL information, replacing a previously hardcoded value. Unit tests were added to verify the new logic. Feedback suggests expanding the test suite to include scenarios where transport details are provided as nested dictionaries, ensuring that the implementation's handling of mapping types is fully covered and protected against regressions.
| [ | ||
| ({"transport": "Streamable HTTP", "url": "https://example.com/mcp"}, "streamable_http"), | ||
| ({"transport_type": "sse", "url": "https://example.com/mcp"}, "sse"), | ||
| ({"type": "streamable_http", "url": "https://example.com/mcp"}, "streamable_http"), | ||
| ({"url": "https://example.com/mcp/sse"}, "sse"), | ||
| ({"url": "https://example.com/mcp"}, "streamable_http"), | ||
| ], |
There was a problem hiding this comment.
The test coverage for _modelscope_mcp_transport_from_url_info is good, but it's missing cases for when the transport value is a nested dictionary. The implementation at astrbot/core/provider/func_tool_manager.py:152-153 handles this scenario, so it would be beneficial to add test cases for it to prevent future regressions. Specifically, the logic handling Mapping types (checking for type or name keys) is not covered. Please consider adding test cases to cover this path.
[
({"transport": "Streamable HTTP", "url": "https://example.com/mcp"}, "streamable_http"),
({"transport_type": "sse", "url": "https://example.com/mcp"}, "sse"),
({"type": "streamable_http", "url": "https://example.com/mcp"}, "streamable_http"),
({"transport": {"type": "sse"}, "url": "https://example.com/mcp"}, "sse"),
({"protocol": {"name": "Streamable HTTP"}, "url": "https://example.com/mcp"}, "streamable_http"),
({"url": "https://example.com/mcp/sse"}, "sse"),
({"url": "https://example.com/mcp"}, "streamable_http"),
],References
- New functionality should be accompanied by corresponding unit tests.
1 participant
Summary
/sseURL fallback casesFixes #8269
Test Plan
uv run pytest tests/unit/test_func_tool_manager.py::test_modelscope_mcp_transport_from_url_info -q --basetemp .tmp/pytest-modelscope-transport -o cache_dir=.tmp/pytest-cache-modelscopeuv run pytest tests/unit/test_func_tool_manager.py -q --basetemp .tmp/pytest-func-tool-manager -o cache_dir=.tmp/pytest-cache-func-tool-manageruv run ruff check astrbot/core/provider/func_tool_manager.py tests/unit/test_func_tool_manager.pypython -m py_compile astrbot/core/provider/func_tool_manager.py tests/unit/test_func_tool_manager.pygit diff --checkSummary by Sourcery
Preserve and infer the correct transport type for synced ModelScope MCP servers based on provided metadata or URL patterns.
Bug Fixes:
Enhancements:
Tests: